Skip to content

Visual Studio warning fixes and improvements#27939

Merged
cbjeukendrup merged 1 commit into
musescore:masterfrom
krasko78:VisualStudioWarningFixesAndImprovements
May 5, 2025
Merged

Visual Studio warning fixes and improvements#27939
cbjeukendrup merged 1 commit into
musescore:masterfrom
krasko78:VisualStudioWarningFixesAndImprovements

Conversation

@krasko78

@krasko78 krasko78 commented May 4, 2025

Copy link
Copy Markdown
Contributor

This PR:

  1. Stops "defaultlib 'LIBCMT' conflicts with use of other libs" linker warning on all EXE projects
    Also stop "M_LN2 macro redefinition" compiler warning.
    image

  2. Adds a post-build event to the MuseScoreStudio project to deploy the app to the output folder (same as what the INSTALL target does). This was a missing step that every Visual Studio user struggled with individually. This change makes the INSTALL target, when built, deploy the app twice but everything will already be up-to-date and this is a much less severe thing than the code changes not getting automatically deployed when building MuseScore.

  3. Adds the possibility of having a SetupBuildEnvironment.user.cmake file in the same folder as SetupBuildEnvironment.cmake to allow individual users to override settings as they see fit. Some recent changes have made the Debug build configuration in Visual Studio less pleasant to use and switching to RelWithDebInfo also comes with compromises. I think this per-user file will give users more flexibility. Of course, if there is an agreement that a user override change is good enough for all Visual Studio users, it will be included in the global SetupBuildEnvironment.cmake.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@krasko78
krasko78 force-pushed the VisualStudioWarningFixesAndImprovements branch 5 times, most recently from 9d37206 to 969e097 Compare May 4, 2025 18:58
@krasko78
krasko78 marked this pull request as draft May 4, 2025 19:00
Comment thread src/app/CMakeLists.txt Outdated
# Add a post-build event to deploy the application to the output folder. Same as building the INSTALL target.
#add_custom_command(
# TARGET MuseScoreStudio POST_BUILD
# COMMAND ${CMAKE_COMMAND} -DBUILD_TYPE=$(Configuration) -P cmake_install.cmake

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ${Configuration}?, curly braces?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't want the value of the CMake variable Configuration here. Instead we want $(Configuration) to go verbatim to the post-build event where Visual Studio when running it will replace $(Configuration) with the current build configuration. Ninja does not like the $ but why? We have the same pattern elsewhere. I am trying to see if putting it inside double quotes will make everyone happy. Or doubling the $.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it's too late now, but it might have worked for this to be the CMake generator expression $<CONFIG> :)

@Jojo-Schmitz Jojo-Schmitz May 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too late to put that into a new PR ;-)

BTW: any idea how to do a change to the same effect for 3.x? A simple copy/paste does not work
(see my attempt in Jojo-Schmitz#931)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my reply over there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was particularly thick here...

@krasko78
krasko78 force-pushed the VisualStudioWarningFixesAndImprovements branch 11 times, most recently from 4b9180d to 93068a1 Compare May 5, 2025 00:14
@krasko78
krasko78 marked this pull request as ready for review May 5, 2025 01:01
Comment thread src/app/CMakeLists.txt Outdated
Comment thread src/app/CMakeLists.txt Outdated
@krasko78
krasko78 force-pushed the VisualStudioWarningFixesAndImprovements branch from 93068a1 to d956d3d Compare May 5, 2025 11:55
@krasko78

krasko78 commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

Thanks Casper. What is needed to merge it? I am wondering if we want to make the "SetupBuildEnvironment.user.cmake" change for everyone? Currently it is for MSVC only.

@cbjeukendrup

Copy link
Copy Markdown
Collaborator

I was waiting for the checks to finish, but they have done so by now.

But yes, perhaps it's a good idea to move that if/include to the end of the file, so that it can be used with all compilers.

Now that we're discussing it anyway, what are you currently using it for? (Just wondering if it's something we should maybe consider doing by default for everyone)

@krasko78

krasko78 commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

I added it because I want to disable all optimizations in my RelWithDebInfo configuration.

@cbjeukendrup

Copy link
Copy Markdown
Collaborator

Ah okay. Anyway, for consistency, let's make it also available with non-MSVC, and then I'll merge it.

@krasko78
krasko78 force-pushed the VisualStudioWarningFixesAndImprovements branch from d956d3d to add5d9f Compare May 5, 2025 15:40
@krasko78

krasko78 commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

Updated. Many thanks!

@cbjeukendrup
cbjeukendrup merged commit 7f27a3f into musescore:master May 5, 2025
@krasko78
krasko78 deleted the VisualStudioWarningFixesAndImprovements branch May 6, 2025 23:41
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2025
reg.: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library (LNL4098)
Esp. (only?) in Debug builds.

(Partial) Backport of musescore#27939
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2025
reg.: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library (LNK4098), esp. (only?) in Debug builds.

(Partial) Backport of musescore#27939
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2025
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2025
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jun 14, 2026
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants